Conversation
|
@ragingrahul is attempting to deploy a commit to the Abacus Works Team on Vercel. A member of the Team first needs to authorize it. |
jmrossy
left a comment
There was a problem hiding this comment.
Thanks @ragingrahul for the PR! This is a good start but it's incomplete and a bit sloppy. Is there a way to use the Tenderly APIs without a user's secret access key? If not, how did you intend to get that from the user?
package.json
Outdated
| "@metamask/jazzicon": "https://github.com/jmrossy/jazzicon#7a8df28974b4e81129bfbe3cab76308b889032a6", | ||
| "@rainbow-me/rainbowkit": "^0.11.0", | ||
| "@tanstack/react-query": "^4.24.10", | ||
| "axios": "^1.4.0", |
There was a problem hiding this comment.
Please use the browser native fetch API, no need to add a new library just for a POST request :)
| function CallDataModal({ debugResult }: { debugResult?: MessageDebugResult }) { | ||
| function CallDataModal({ debugResult,chainId }: { debugResult?: MessageDebugResult,chainId:ChainId }) { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const disabled=false |
There was a problem hiding this comment.
Why do you have a var here if it never changes?
| <button onClick={async()=>{ | ||
| handleClick() | ||
| await simulateCall({contract,handleCalldata,chainId}) |
There was a problem hiding this comment.
Move this line to the click handler
| const [buttonText,setButtonText]=useState<string>("Simulate Handle Call") | ||
| if (!debugResult?.calldataDetails) return null; | ||
| const { contract, handleCalldata } = debugResult.calldataDetails; | ||
| function handleClick() { |
There was a problem hiding this comment.
event handlers (like click) should use lambda (arrow fn) syntax to be bound correctly.
| function CallDataModal({ debugResult,chainId }: { debugResult?: MessageDebugResult,chainId:ChainId }) { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const disabled=false | ||
| const [buttonText,setButtonText]=useState<string>("Simulate Handle Call") |
There was a problem hiding this comment.
The <string> type is inferred so it's unnecessary here.
There was a problem hiding this comment.
Change to Simulate call with Tenderly
| save_if_fails: true, | ||
| simulation_type: 'full', | ||
| network_id: chainId, | ||
| from: '0xdc6bdc37b2714ee601734cf55a05625c9e512461',//can be any address, doesn't matter |
There was a problem hiding this comment.
Use the 0 address if you just need a placeholder (ethers.constants.zeroAddress)
| gas: 8000000, | ||
| gas_price: 0, | ||
| value: 0, |
There was a problem hiding this comment.
You'll need to use the real values from the message for an accurate simulation.
| } | ||
| ); | ||
| console.timeEnd('Simulation'); | ||
| console.log(resp.data) |
There was a problem hiding this comment.
Use the logger utility instead of console and add a message to provide context for the data you're logging
| } | ||
| ) | ||
| console.log(share) | ||
| window.location.assign(`https://dashboard.tenderly.co/shared/simulation/${resp.data.simulation.id}`) |
There was a problem hiding this comment.
Please open the tenderly window in a new tab
| setDisabled(false) | ||
| setShowSpinner(false) |
There was a problem hiding this comment.
The disabled state always equals the showSpinner state so there's no reason to create 2 state vars. Can you please use the replace them with a single [loading, setLoading] state that is the inverse of disabled now?
| from: '0x0000000000000000000000000000000000000000',//can be any address, doesn't matter | ||
| to: contract, | ||
| input:handleCalldata, | ||
| gas: Number(message.totalGasAmount), |
There was a problem hiding this comment.
Please use BigNumber.from(message.totalGasAmount).toNumber()
| to: contract, | ||
| input:handleCalldata, | ||
| gas: Number(message.totalGasAmount), | ||
| gas_price: Number(message.totalPayment)/Number(message.totalGasAmount), |
There was a problem hiding this comment.
Use the computeAvgGasPrice util function here. It handles possible rounding issues:
https://github.com/hyperlane-xyz/hyperlane-explorer/blob/main/src/features/messages/cards/GasDetailsCard.tsx#L168
| value: 0, | ||
| } | ||
| const resp=await fetch( | ||
| `https://explorer.hyperlane.xyz/api/simulation`,{ |
There was a problem hiding this comment.
Remove the https://explorer.hyperlane.xyz prefix. It's implied when coming from the same domain and will break the feature in dev builds
src/pages/api/simulation.ts
Outdated
| const data=req.body | ||
|
|
||
| const resp = await fetch( | ||
| `https://api.tenderly.co/api/v1/account/${TENDERLY_USER}/project/${TENDERLY_PROJECT}/simulate`, |
There was a problem hiding this comment.
Check that these env vars are defined and throw a helpful here at the top if they're not
src/pages/api/simulation.ts
Outdated
| }, | ||
| } | ||
| ) | ||
| res.json(simulationId) |
There was a problem hiding this comment.
Wrap the result in a success/failure result shape: https://github.com/hyperlane-xyz/hyperlane-explorer/blob/main/src/features/api/utils.ts
|
|
||
|
|
There was a problem hiding this comment.
Spacing? Please run yarn prettier and the files will get auto-formatted :)
|
|
||
| } | ||
|
|
||
| function computeAvgGasPrice(unit: string, gasAmount?: BigNumber.Value, payment?: BigNumber.Value) { |
There was a problem hiding this comment.
Why did you duplicate this function here? Please de-dupe with the copy in GasDetailsCard and move it to messages/utils.ts
| ); | ||
| } | ||
| async function simulateCall({contract,handleCalldata,chainId,message}:{contract:string,handleCalldata:string,chainId:ChainId,message:Message}){ | ||
|
|
There was a problem hiding this comment.
Wrap the function contents here in a try/catch and show an error message with toast.error on failure
There was a problem hiding this comment.
Didn't wrap this in try/catch, rather wrapped the backend call which returns failure call and checking if the call is success or failure here.
jmrossy
left a comment
There was a problem hiding this comment.
Code looks okay, just one small comment, but there should be no yarn.lock file changes. That's likely why the CI is failing.
src/pages/api/simulation.ts
Outdated
| ) | ||
| res.json(successResult(simulationId)) | ||
| } catch (error) { | ||
| res.json(failureResult("Could not simulate")) |
There was a problem hiding this comment.
Please change to "Error preparing Tenderly simluation"
| export default async function handler(req, res) { | ||
| const data = req.body; | ||
| if (!TENDERLY_ACCESS_KEY || !TENDERLY_PROJECT || !TENDERLY_USER) { | ||
| console.log('ENV not defined'); |
There was a problem hiding this comment.
Use logger instead of console, I believe this will raise a linter warning
There was a problem hiding this comment.
Change log message to something more descriptive: Tenderly key, project, or user not defined in env
| const data = req.body; | ||
| if (!TENDERLY_ACCESS_KEY || !TENDERLY_PROJECT || !TENDERLY_USER) { | ||
| console.log('ENV not defined'); | ||
| res.json(failureResult('Explorer Issues')); |
There was a problem hiding this comment.
Change to 'Tenderly credentials missing'
| </p> | ||
| <LabelAndCodeBlock label="Recipient contract address:" value={contract} /> | ||
| <LabelAndCodeBlock label="Handle function input calldata:" value={handleCalldata} /> | ||
| <button onClick={handleClick} disabled={loading} className="underline text-blue-400"> |
There was a problem hiding this comment.
Show only when not loading (see feedback on discord).
Please also reduce text size: text-sm
Description
Implemented tenderly transaction simulation
To Know
TENDERLY_USER,TENDERLY_PROJECT,TENDERLY_ACCESS_KEY can be found USER & PROJECT and ACCESS-KEY
Related Issues
#2488